[ESLint] Prevent `no-html-link-for-pages` from warning for static files #31495

nilskaspersson · 2021-11-16T14:43:28Z

The rule no-html-link-for-pages will incorrectly flag an <a>-tag intended to download a local asset. This PR adds an exception to the rule for any anchor element with a download attribute.

housseindjirdeh

Good catch, thanks for submitting this fix :)

ijjk

This should only match if the asset is a page. The rule shouldn't show an error if /static-file.csv is a public file already. Are you seeing the detection here

next.js/packages/eslint-plugin-next/lib/rules/no-html-link-for-pages.js

Line 82 in d550b13

const urls = getUrlFromPagesDirectories('/', foundPagesDirs)

incorrectly showing for a non-page?

nilskaspersson · 2021-11-17T08:35:47Z

This should only match if the asset is a page. The rule shouldn't show an error if /static-file.csv is a public file already. Are you seeing the detection here

next.js/packages/eslint-plugin-next/lib/rules/no-html-link-for-pages.js

Line 82 in d550b13

const urls = getUrlFromPagesDirectories('/', foundPagesDirs)

incorrectly showing for a non-page?

Yes, I encountered this while working. Specifically after upgrading to Next 12. The added test failed too, until the exception was made.

The return value of getUrlFromPagesDirectories() in the test suite is:

[
  /^\/.*\/$/,
  /^\/.*\/index\/$/,
  /^\/$/,
  /^\/index\/$/,
  /^\/list\/.*\/$/
]

In my application, the return value contains the same generic rules (excluding the list pattern) and, of course, my pages.

The issue seems to be the normalizeURL util appending a trailing slash to the provided value, making any relative link match the first pattern. I do not understand why that pattern (or trailing slashes, for that matter) is necessary, so I cannot say if something is wrong here. Removing either the trailing slash or the first pattern would satisfy the test, but a deliberate exception for the use case seems better to me.

ijjk · 2021-11-19T03:05:17Z

Ah I see the first regex there seems to be causing any href at the top-level to be matched which makes sense in most cases since there's a dynamic route at the base of pages although it should probably not match static files as those should not use next/link. I added an extra change to prevent dynamic routes from matching static file links in the lint rule even without the download attribute being present. Thanks for investigating this!

ijjk · 2021-11-19T03:17:12Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall decrease ✓

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
buildDuration	16.4s	16.3s	-150ms
buildDurationCached	3.4s	3.2s	-200ms
nodeModulesSize	339 MB	339 MB	-4 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.618	2.715	⚠️ +0.1
/ avg req/sec	955.03	920.91	⚠️ -34.12
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.266	1.245	-0.02
/error-in-render avg req/sec	1975.22	2007.94	+32.72

Client Bundles (main, webpack, commons)

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	28.3 kB	28.3 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	72.2 kB	72.2 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
_app-HASH.js gzip	1.37 kB	1.37 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	2.38 kB	2.38 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	635 B	635 B	✓
image-HASH.js gzip	4.45 kB	4.45 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	1.87 kB	1.87 kB	✓
routerDirect..HASH.js gzip	321 B	321 B	✓
script-HASH.js gzip	383 B	383 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	13.3 kB	13.3 kB	✓

Client Build Manifests

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
index.html gzip	523 B	523 B	✓
link.html gzip	535 B	535 B	✓
withRouter.html gzip	516 B	516 B	✓
Overall change	1.57 kB	1.57 kB	✓

Default Build with SWC (Increase detected ⚠️)

General Overall decrease ✓

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
buildDuration	18.1s	17.8s	-268ms
buildDurationCached	3.3s	3.6s	⚠️ +318ms
nodeModulesSize	339 MB	339 MB	-4 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.662	2.666	0
/ avg req/sec	939.06	937.73	⚠️ -1.33
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.347	1.252	-0.09
/error-in-render avg req/sec	1856.15	1996.39	+140.24

Client Bundles (main, webpack, commons)

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.6 kB	28.6 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	72.5 kB	72.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	305 B	305 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.38 kB	2.38 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	622 B	622 B	✓
image-HASH.js gzip	4.47 kB	4.47 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	1.91 kB	1.91 kB	✓
routerDirect..HASH.js gzip	314 B	314 B	✓
script-HASH.js gzip	375 B	375 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	13.2 kB	13.2 kB	✓

Client Build Manifests

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	nilskaspersson/next.js no-html-link-for-pages-download-exception	Change
index.html gzip	524 B	524 B	✓
link.html gzip	535 B	535 B	✓
withRouter.html gzip	517 B	517 B	✓
Overall change	1.58 kB	1.58 kB	✓

Commit: 78b859f

nilskaspersson added 2 commits November 16, 2021 15:07

Do not trigger for <a> tags with download attr

daad3d7

Test for valid download link use case

2ac08e3

nilskaspersson requested review from huozhi, ijjk, shuding and timneutkens as code owners November 16, 2021 14:43

ijjk added the type: next label Nov 16, 2021

housseindjirdeh previously approved these changes Nov 16, 2021

View reviewed changes

ijjk reviewed Nov 16, 2021

View reviewed changes

ijjk added 2 commits November 18, 2021 20:56

Prevent matching dynamic route against static file for lint rule

2db8744

Merge branch 'canary' into no-html-link-for-pages-download-exception

78b859f

ijjk dismissed housseindjirdeh’s stale review via 78b859f November 19, 2021 03:00

ijjk changed the title ~~[ESLint] Add exception for <a> tag with download attr to no-html-link-for-pages~~ [ESLint] Prevent no-html-link-for-pages from warning for static files Nov 19, 2021

ijjk approved these changes Nov 19, 2021

View reviewed changes

kodiakhq bot merged commit d1ce7b7 into vercel:canary Nov 19, 2021

nilskaspersson deleted the no-html-link-for-pages-download-exception branch November 19, 2021 08:40

vercel locked as resolved and limited conversation to collaborators Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ESLint] Prevent `no-html-link-for-pages` from warning for static files #31495

[ESLint] Prevent `no-html-link-for-pages` from warning for static files #31495

nilskaspersson commented Nov 16, 2021

housseindjirdeh left a comment

ijjk left a comment

nilskaspersson commented Nov 17, 2021 •

edited

Loading

ijjk commented Nov 19, 2021

ijjk commented Nov 19, 2021

[ESLint] Prevent no-html-link-for-pages from warning for static files #31495

[ESLint] Prevent no-html-link-for-pages from warning for static files #31495

Conversation

nilskaspersson commented Nov 16, 2021

housseindjirdeh left a comment

Choose a reason for hiding this comment

ijjk left a comment

Choose a reason for hiding this comment

nilskaspersson commented Nov 17, 2021 • edited Loading

ijjk commented Nov 19, 2021

ijjk commented Nov 19, 2021

Stats from current PR

[ESLint] Prevent `no-html-link-for-pages` from warning for static files #31495

[ESLint] Prevent `no-html-link-for-pages` from warning for static files #31495

nilskaspersson commented Nov 17, 2021 •

edited

Loading